Skip to content

Default draft protocol support: sessionless + handshake-less (SEP-2575 + SEP-2567)#1610

Open
halter73 wants to merge 16 commits into
mainfrom
halter73/remove-session-id-draft
Open

Default draft protocol support: sessionless + handshake-less (SEP-2575 + SEP-2567)#1610
halter73 wants to merge 16 commits into
mainfrom
halter73/remove-session-id-draft

Conversation

@halter73

@halter73 halter73 commented May 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the draft MCP protocol revision (2026-07-28) in the C# SDK — removing the initialize handshake and Mcp-Session-Id per SEP-2575 and SEP-2567, while preserving back-compat with legacy clients/servers via probe-and-fallback negotiation.

Stacked on the now-merged #1458 (MRTR). Opt in to draft by setting ProtocolVersion = McpSessionHandler.DraftProtocolVersion.

What's in

Protocol

  • DraftProtocolVersion value set to "2026-07-28" (spec string, replaces MRTR's "DRAFT-2026-v1" placeholder).
  • server/discover registered on every server; serves as the bootstrap mechanism (clients send it first under draft).
  • Per-request _meta.io.modelcontextprotocol/protocolVersion is validated server-side; unsupported versions return -32004 UnsupportedProtocolVersionError with {supported, requested} data.
  • ttlMs + cacheScope added to DiscoverResult per spec PR #2855; defaults to ttlMs: 0 + cacheScope: "private" under draft (immediate-stale, not shareable) for safe back-compat behavior.

Transport

  • HttpServerTransportOptions.Stateless defaults to true for new code.
  • Stateful-only knobs (EventStreamStore, SessionMigrationHandler, PerSessionExecutionContext, IdleTimeout, MaxIdleSessionCount, plus ISseEventStreamStore / ISessionMigrationHandler) are marked [Obsolete(MCP9005)] — see docs/list-of-diagnostics.md.
  • Under draft + no session-id, the HTTP handler forces stateless per request.

Client negotiation

  • HTTP fallback: client sends draft request first. On 400 Bad Request it parses the body — modern JSON-RPC errors (-32004, -32003, -32001) surface as McpProtocolException to the caller; any other JSON-RPC error (legacy -32600, -32601, -32700, parse fail, empty body) → switch to legacy and initialize. Matches spec PR #2844 ("the fallback MUST NOT be keyed to a single error code").
  • Stdio fallback: client probes with server/discover first. DiscoverResult → modern. -32004 with shaped data → retry with supported[]. Anything else, or silence past the 5-second probe timeout → fall back to initialize on the same stdin/stdout (no process restart per spec).
  • AutoDetectingClientSessionTransport now recognizes JSON-RPC error envelopes in HTTP 400 bodies; adopts StreamableHttp instead of silently falling back to SSE on modern-error responses.

Public API

  • McpClientOptions.MinProtocolVersion : string? — when set, the client refuses to fall back below this version and surfaces a clear McpException instead. Useful for strict-modern production code and for tests that want to assert draft-only behavior.

What's tested

  • Existing draft & MRTR coverage: all 51 draft/MRTR/raw-stream tests green.
  • New raw conformance tests:
    • tests/ModelContextProtocol.Tests/Server/RawStreamConformanceTests.cs — drives McpServer directly via paired Pipe streams without McpClient. 5 tests covering server/discover first, draft tools/call after no init, -32004 on unsupported version, legacy initialize still works, dual-era dispatch on the same stream.
    • tests/ModelContextProtocol.AspNetCore.Tests/RawHttpConformanceTests.cs — drives the C# server with raw HttpClient against in-memory Kestrel. 5 tests covering draft tools/call with full _meta, raw server/discover, -32004 on unsupported MCP-Protocol-Version header, legacy initialize on the default (stateless+draft) server, and GET /mcp returning 405 when not stateful.
  • New HTTP fallback regression tests (tests/ModelContextProtocol.AspNetCore.Tests/DraftHttpFallbackTests.cs): three Kestrel-in-memory cases covering Python-shape (-32600 legacy error), Go-shape (-32004 with supported data), and HeaderMismatch-shape (-32001) on real HTTP. Plus null-id parser tests and HeaderMismatch passthrough test on the in-memory transport.
  • HTTP+draft → stateless migration: tests that relied on stateful HTTP for legacy coverage (notably HttpTaskIntegrationTests) now explicitly opt into Stateless = false.
  • Cross-suite status at tip d9277e0c:
    • Core (ModelContextProtocol.Tests, net9.0, excluding env-dependent ClientIntegrationTests / DockerEverythingServerTests and the env-quirk-only StdioClientTransportTests.EscapesCliArgumentsCorrectly which depends on local PATH/CMD.EXE config): 2052 passed / 4 skipped. The full suite reports 72 fails for EscapesCliArgumentsCorrectly, all on a parameterized test that's git diff origin/main..HEAD = 0 (i.e. unchanged in this PR); CI on main is green.
    • AspNetCore (ModelContextProtocol.AspNetCore.Tests, net9.0): 482 passed / 3 failed / 29 skipped. The 3 failures are all pre-existing on main: Server_RejectsInvalidUtf8EncodedHeaderValue, RunConformanceTest_Sep2243("http-custom-headers") (the SEP-2243 finding below), and RunCachingConformanceTest (parallel-run port collision; passes in 1s in isolation).

Cross-SDK compatibility (Phase 7 + Phase 11d)

Validated against the other Tier-1 SDKs (TypeScript, Python, Go) in their current main / draft-branch states. Wire-trace artifacts kept in this branch's session state.

C# direction Peer Transport Status
→ TS draft (PR #2251 sep-2575-2567-draft-protocol) http PASS — draft handshake-less, tools/list succeeds
→ Python simple-streamablehttp-stateless (origin/main) http PASS — C# probes draft, recognizes -32600, falls back to legacy initialize, negotiates 2025-06-18
→ Python simple-tool (origin/main) stdio PASS — C# probes server/discover, gets -32601, falls back to initialize on the same stdin/stdout, negotiates 2025-06-18
→ Go vanilla HTTP (origin/main, AutoDetect default) http PASS — AutoDetect recognizes -32004 in 400 body, adopts StreamableHttp, retries legacy initialize with 2025-11-25, lists 10 tools
→ Go vanilla stdio (origin/main, after PR #987) stdio PASS — Go responds to server/discover natively; C# negotiates down to 2025-11-25
→ Go draft fork (compat/go-draft-fork with version-string + exported opt-in patches) both PASS — full modern draft session end-to-end
← TS draft client both PASS — C# server serves server/discover and tools/list
← Go draft fork client both PASS
← Python simple-tool client stdio PASS — Python sends legacy initialize with max 2025-06-18; C# server (stateless default) serves single-shot legacy session

α-findings fixed in this PR (post-cross-SDK testing)

Commit Fix
ccdd4223 Accept null-id JSON-RPC error responses per spec §5.1 (Python's simple-streamablehttp-stateless returns id: null on errors before the request id can be determined).
00d57f71 Surface any JSON-RPC error in HTTP 400 body as McpProtocolException per spec PR #2844 (not just modern -32004/-32003) so the connect-time fallback chain can dispatch on the error code.
276bde45 Surface HeaderMismatch (-32001) instead of falling back to legacy initialize.
3778e00e AutoDetectingClientSessionTransport now recognizes JSON-RPC error envelopes in HTTP 400 bodies; adopts StreamableHttp instead of silently falling back to SSE.

β-findings (peer-SDK issues, informational)

  • β-TS-1 — TS draft client (compat/ts-draft) doesn't yet emit Mcp-Method / Mcp-Name headers (the fix is on a different branch). Closure awaits upstream merge.
  • β-PY-1 — partially closed; Python main no longer crashes on draft probe, now returns clean JSON-RPC error envelope.
  • β-GO-1, β-GO-2 — Go origin/main still uses 2026-06-30 version string and unexported ClientSessionOptions.protocolVersion. Documented; patches applied locally for cross-SDK testing only.

Conformance suite (Phase 12)

Ran the upstream @modelcontextprotocol/conformance suite against the C# SDK. Two tracks:

Track A — bump the published npm pin

Bumped tests/Common/Utils/package.json from 0.1.160.2.0-alpha.2 (d539e7fd). This activates 5 previously-gated test classes (ClientConformanceTests.RunConformanceTest_Sep2243, ServerConformanceTests.RunConformanceTest_HttpHeaderValidation, ServerConformanceTests.RunConformanceTest_HttpCustomHeaderServerValidation, ServerConformanceTests.RunMrtrConformanceTest, CachingConformanceTests.RunCachingConformanceTest).

Because 0.2.0-alpha.2 still emits the placeholder wire version DRAFT-2026-v1 (the spec-aligned 2026-07-28 only landed in unpublished alpha.3), a wire-version-match gate (HasMatchingDraftWireVersion() in tests/Common/Utils/NodeHelpers.cs, commit f3698c71) is ANDed into each draft-only HasXxx skip predicate so the 14 draft-scenario rows skip cleanly under the published alpha.2 instead of failing with mismatched-string assertions.

Track B — local build of compat/conformance-draft

Assembled a local compat/conformance-draft branch in modelcontextprotocol/conformance (tip 50ad0fa) by merging the following SEP-relevant open PRs on top of main:

  • #336 — stateless error-code expectations (-32001 / -32004)
  • #262 — Tasks (SEP-2663) MRTR scenarios
  • #333 — SEP-2567 sessionless suite

#310 (SEP-2549 absence-assert) was skipped — too-deep conflict with main's RunContext refactor (PRs #319 / #317 / #321 / #318). Deferred to a follow-up.

Installed locally with npm install --no-save H:\modelcontextprotocol\conformance. ⚠️ Note: npm ci reverts to pinned alpha.2; reviewers reproducing locally must re-run the path-install after dependency restore. Flipped 3 --spec-version DRAFT-2026-v1 references in ServerConformanceTests.cs + 1 in CachingConformanceTests.cs to 2026-07-28 (commit d9277e0c), and renamed 6 tools + 1 prompt in IncompleteResultTools.cs / IncompleteResultPrompts.cs to match conformance's rename of incomplete-result-* → input-required-result-* (mirrors the SDK's MRTR IncompleteResult → InputRequiredResult rename).

Outcome (serial run on stateless HTTP):

Result Count Notes
PASS 19 including all 14 MRTR input-required-result-* scenarios — the tampered-state (HMAC-protected requestState) and capability-check (per-request capability-aware inputRequest gating) rows are now implemented in ConformanceServer and un-skipped — plus both Sep2243.http-{standard,invalid-tool}-headers and Caching
FAIL 0
SKIP / N-A 1 ClientConformanceTests.RunConformanceTest_Sep2243("http-custom-headers")not a C# bug: the harness scenario puts x-mcp-header on a type: "number" parameter, which SEP-2243 forbids (the spec's earlier self-contradiction was resolved against number upstream in modelcontextprotocol/modelcontextprotocol#2863). The C# client correctly excludes the malformed tool, so no Mcp-Param-* headers are sent. Stays skipped until a conformant package ships; tracked in #1655.

Modes: only stateless HTTP exercised so far. Stateful HTTP and stdio modes deferred to a follow-up — Track B already validates draft conformance on the most important transport, and the published-pin gate (Track A) ensures CI on pinned alpha.2 keeps working without local conformance-build dependencies.

Parallel-run flakiness: CachingConformanceTest shows a port-pool collision (port 301x range) under parallel xUnit collections; passes consistently in isolation in under 2 s. Documented as known-flaky-in-parallel; the test suite was not switched to serial.

Out of scope

  • The Tasks extension (SEP-2663) shipped in Implement SEP-2663 Tasks Extension #1579 (merged); this PR rebases on top of it.
  • Legacy protocol types (InitializeRequestParams, Mcp-Session-Id constants, PingRequestParams, …) are still current in 2025-11-25 and remain un-obsoleted in this PR.
  • HTTP+SSE (2024-11-05) transport stays mapped under /sse and /message for legacy back-compat.

Resolved during review (originally punted, now done in this PR)

  • HeaderMismatch (-32001) validation — the server already compared the HTTP MCP-Protocol-Version header against the body _meta.io.modelcontextprotocol/protocolVersion, but threw -32602 InvalidParams. A draft client's server/discover probe treats any non-modern error (including InvalidParams) as a legacy signal and falls back to initialize, so a modern server detecting a genuine mismatch was misread as legacy. It now emits -32001 HeaderMismatch — the code the client recognizes as a modern-server signal — with a RawHttpConformanceTests regression.
  • The two previously-skipped MRTR conformance scenarios (input-required-result-tampered-state HMAC + input-required-result-capability-check per-request gating) are now implemented in ConformanceServer and un-skipped (14/14 RunMrtrConformanceTest pass against the local compat/conformance-draft build), with in-process wire-level regressions in MrtrProtocolTests.

Punted to follow-up PRs

halter73 and others added 8 commits June 15, 2026 17:13
…d (SEP-2575, SEP-2567)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…options (MCP9005)

Default `HttpServerTransportOptions.Stateless` to true so new code on the 2026-07-28
draft revision (SEP-2567) is sessionless from the start. Mark the surface that only
makes sense in the legacy stateful HTTP mode as obsolete behind the new MCP9005
diagnostic so callers see a deprecation hint but can still pin Stateless = false
to keep using session-based behaviors during back-compat:

* `HttpServerTransportOptions.EventStreamStore` (resumability)
* `HttpServerTransportOptions.SessionMigrationHandler` (multi-node migration)
* `HttpServerTransportOptions.PerSessionExecutionContext`
* `HttpServerTransportOptions.IdleTimeout`
* `HttpServerTransportOptions.MaxIdleSessionCount`

Internal infrastructure that legitimately reads those options for the back-compat
stateful path now suppresses MCP9005 at the use site. Test projects suppress it
globally via NoWarn because the suite intentionally exercises both modes.

Update tests/samples that previously relied on the implicit `Stateless = false`
default to set it explicitly:

* TestSseServer.Program — SSE always needs stateful state shared across GET/POST.
* ConformanceServer.Program — resumability + OAuth conformance scenarios are stateful.
* ResumabilityIntegrationTestsBase — resumability is a stateful concern.
* SseIntegrationTests / MapMcpSseTests — SSE requires stateful.
* OAuthTestBase — OAuth flow uses the GET /sse session-based endpoint.
* MrtrProtocolTests / SessionMigrationTests / StreamableHttpServerConformanceTests —
  these tests intentionally drive the legacy stateful session machinery.
* DraftHttpHandlerTests — tests draft rejection of GET/DELETE endpoints, which are
  only mapped when Stateless = false.

Rework HTTP header conformance helpers (HttpHeaderConformanceTests +
StreamableHttpServerConformanceTests) to stop asserting an mcp-session-id response
header from draft/non-draft initialize, because the sessionless default means none
is returned.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…to legacy protocol

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…of overwriting

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Detect fallback

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… #2855)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…pec-version strings

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- RawStreamConformanceTests.cs: wrap in #if !NET472 to avoid
  ReadLineAsync(CancellationToken) overload missing on .NET Framework.
- HttpMcpServerBuilderExtensionsTests: IdleTrackingBackgroundService_StartsTimer_WhenStateful
  needs explicit Stateless=false after the default flipped to true in commit 8904958.
- HttpHeaderConformanceTests: two tests used the old DRAFT-2026-v1 wire-version
  string which the server now rejects; updated to 2026-07-28.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@halter73 halter73 force-pushed the halter73/remove-session-id-draft branch from d9277e0 to f3f6843 Compare June 16, 2026 00:23
@halter73 halter73 changed the title Add draft protocol support: sessionless + handshake-less (SEP-2575 + SEP-2567) Default draft protocol support: sessionless + handshake-less (SEP-2575 + SEP-2567) Jun 16, 2026
halter73 and others added 2 commits June 15, 2026 18:08
The server validated that Mcp-Param-* header values are conformantly encoded
(printable ASCII, or a "=?base64?...?=" wrapper for non-ASCII) but applied no
such validation to the standard Mcp-Name header. Raw non-ASCII Mcp-Name values
were passed through and compared byte-for-byte against the body name.

Mirror the existing Mcp-Param-* validation for Mcp-Name: reject values
containing characters outside the valid HTTP header value range, then decode
the "=?base64?...?=" wrapper before comparing to the body value. This makes the
server reject mis-encoded non-ASCII names and correctly accept compliant
base64-wrapped non-ASCII tool/resource/prompt names.

Fixes HttpHeaderConformanceTests.Server_RejectsInvalidUtf8EncodedHeaderValue,
which previously passed only incidentally on the stateful draft path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The draft 2026-07-28 protocol removes stream resumability entirely
(a dropped connection is treated as cancellation), so the SSE event
stream store surface is legacy-only. Mark the resumability interfaces,
options, and wire-up [Obsolete] under the existing MCP9005
(LegacyStatefulHttp) diagnostic, matching the already-obsoleted stateful
HTTP options:

- ISseEventStreamReader / ISseEventStreamWriter / ISseEventStreamStore
- SseEventStreamMode / SseEventStreamOptions
- StreamableHttpServerTransport.EventStreamStore
- DistributedCacheEventStreamStoreOptions
- WithDistributedCacheEventStreamStore

Internal SDK usage of these now-obsolete types is suppressed with
targeted MCP9005 pragmas (and project-level NoWarn where source
generators emit code over the obsolete types). External consumers still
receive the obsolete warning. Behavior is unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@halter73 halter73 marked this pull request as ready for review June 16, 2026 15:08
Under the draft revision (SEP-2575 + SEP-2567) the HTTP request lifetime is
the request lifetime: there are no sessions, so a dropped connection is
equivalent to cancelling the in-flight request. Verify that aborting the HTTP
request flows cancellation into a running tool handler's CancellationToken,
covering both draft sessionless mode and legacy stateless mode (both are 1:1
request-to-handler).

The tests drive raw HTTP via the in-memory Kestrel transport: a tool blocks on
its injected CancellationToken, the client aborts the request mid-flight, and
the tests assert the server observes RequestAborted and the tool's token fires.
No production change was required; the existing session-disposal path already
propagates the abort. These pin that behavior going forward.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
halter73 and others added 3 commits June 16, 2026 09:48
…scenarios

Adds the two SEP-2322 ConformanceServer tools that RunMrtrConformanceTest
previously skipped as "not yet implemented":

- test_input_required_result_tampered_state: R1 issues an HMAC-signed
  requestState; R2 with a tampered requestState surfaces a -32602 JSON-RPC
  error (McpProtocolException propagates as a protocol error, not an isError
  CallToolResult).
- test_input_required_result_capabilities: emits inputRequests only for the
  capabilities the client declared on the per-request _meta clientCapabilities
  envelope (read via JsonRpcMessageContext.ClientCapabilities).

Removes the per-row Skip from both [InlineData] rows so they run under the
same HasMrtrScenarios() gate as the other MRTR scenarios. Verified live:
14/14 RunMrtrConformanceTest scenarios pass against the local
compat/conformance-draft build (which emits the 2026-07-28 wire string).

Adds in-process wire-level regression coverage in MrtrProtocolTests
(TamperedRequestState_ReturnsJsonRpcError and
CapabilityCheck_OnlyEmitsInputRequestsForDeclaredCapabilities) so both
behaviors stay verified in CI even while the published conformance package's
draft wire string lags this SDK.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…atch

When a draft request's MCP-Protocol-Version header disagrees with the
per-request _meta io.modelcontextprotocol/protocolVersion value (SEP-2575),
the server already rejected the request in PopulateContextFromMeta, but it
used -32602 InvalidParams. A conformant draft client's server/discover probe
treats any non-modern JSON-RPC error (including InvalidParams) as a
legacy-server signal and falls back to the initialize handshake. That means a
modern draft server that detected a genuine header/body mismatch would be
misread as legacy.

Emit -32001 HeaderMismatch instead -- the same code already used for the
Mcp-Method/Mcp-Name header-vs-body checks and the exact code the client's
probe recognizes as a modern-server signal to surface as-is (see
McpClientImpl's catch (McpProtocolException ex) when (ex.ErrorCode ==
McpErrorCode.HeaderMismatch)). Adds a RawHttpConformanceTests regression
asserting a header/_meta protocol-version mismatch yields -32001.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Flip McpClientImpl.ConnectAsync so a null ProtocolVersion (the default)
prefers the draft revision (SEP-2575 + SEP-2567): the client probes with
server/discover and transparently falls back to the legacy initialize
handshake when the server doesn't support draft. The legacy branch now runs
only when the caller explicitly pins a non-draft version, making draft
opt-out rather than opt-in.

Merge (rather than overwrite) the session-level client capabilities into each
request's _meta envelope so per-request opt-ins already written by higher
layers (e.g. the tasks-extension capability from GetMetaWithTaskCapability)
survive now that draft _meta injection is the default path. Refresh the XML
docs on McpClientOptions.ProtocolVersion / MinProtocolVersion,
McpSession.DraftProtocolVersion, and McpSessionHandler.DraftProtocolVersion
to describe the new default.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
halter73 and others added 2 commits June 16, 2026 20:20
docs/concepts/stateless/stateless.md references
xref:list-of-diagnostics#obsolete-apis, but list-of-diagnostics.md had no uid
front matter, so `make generate-docs` (docfx --warningsAsErrors) failed on
every CI job that reached it. Add the uid following the existing stateless.md
pattern; the ## Obsolete APIs heading already resolves to the obsolete-apis
anchor.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
With the client default flipped to draft, every test that builds a default
client now negotiates server/discover instead of initialize. Sweep the suite:
tests whose purpose is the legacy initialize handshake, Mcp-Session-Id
lifecycle, session resumption/reconnect, DELETE, event-stream polling, or
server->client sampling over a persistent stream are pinned to 2025-11-25
(these behaviors don't exist under the sessionless draft revision);
handshake-agnostic tests run on the draft default with incidental assertions
(message counts, captured initialize requests, session-id headers) adjusted.

The ConformanceClient pins the legacy "initialize" and "sse-retry" scenarios
while letting the others exercise the draft probe plus transparent legacy
fallback. Draft sampling/elicitation coverage is retained via the stdio MRTR
tests (ClientServerTestBase / MapMcpTests.Mrtr).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@mikekistler mikekistler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

Copilot found a few things worth mentioning. I'll leave it to you to decide if any of these require changes.

// legacy server that silently drops unknown methods (per stdio.mdx fallback rules).
// The probe timeout is bounded by InitializationTimeout, but we cap it at 5s so we
// can quickly fall back when a server isn't going to respond.
var probeTimeout = TimeSpan.FromSeconds(5);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 5-second probeTimeout for server/discover is hardcoded with no option to configure it. For high-latency environments (e.g., cold-start serverless, satellite links), 5s may be too aggressive and could falsely trigger the legacy fallback. Consider exposing this as a property on McpClientOptions or at least making it a named constant with a doc comment explaining the rationale.

/// <summary>Tracks an active <c>subscriptions/listen</c> subscription for notification fan-out.</summary>
private sealed record ActiveSubscription(RequestId Id, SubscriptionsListenNotifications Granted, LoggingLevel? LogLevel);

private readonly ConcurrentDictionary<RequestId, ActiveSubscription> _activeSubscriptions = new();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _activeSubscriptions dictionary is populated here and removed on cancellation, but I don't see code that actually routes notifications to matching subscriptions (e.g., sending tools/list_changed only to subscriptions that have toolsListChanged = true). Is the fan-out implemented elsewhere, deferred to a follow-up PR, or am I missing where it happens?

if (!string.Equals(_negotiatedProtocolVersion, protocolVersion, StringComparison.Ordinal))
{
_negotiatedProtocolVersion = protocolVersion;
_sessionHandler.NegotiatedProtocolVersion = protocolVersion;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under draft protocol, concurrent requests can each carry different per-request _meta/protocolVersion values. The writes to _negotiatedProtocolVersion and _sessionHandler.NegotiatedProtocolVersion here are not atomic with each other and not synchronized against concurrent reads from notification/telemetry code. Is this intentional (last-writer-wins is acceptable for telemetry), or should this be guarded?

Comment on lines +204 to +219
if (context.ClientCapabilities is { } clientCapabilities && IsStatefulSession())
{
// Defensive merge instead of overwrite. SEP-2575 says the per-request envelope is
// the client's full capabilities, but PR #1579's GetMetaWithTaskCapability emits a
// partial envelope (only extensions.io.modelcontextprotocol/tasks) on every
// tools/call regardless of the negotiated protocol version. If we overwrote here,
// a legacy client that called initialize with { Elicitation = new() } would lose
// its elicitation capability the moment it issued a tools/call. Merging non-null
// fields preserves whatever the initialize handshake (or a prior, more complete
// envelope) established.
//
// The IsStatefulSession() gate prevents leaking per-request capability state into
// _clientCapabilities under StreamableHttpServerTransport { Stateless = true }
// (where _clientCapabilities is otherwise null and StatelessServerTests rely on
// that invariant to surface the "X is not supported in stateless mode" errors).
_clientCapabilities = MergeClientCapabilities(_clientCapabilities, clientCapabilities);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment explains why merging is necessary (partial envelopes from the Tasks extension), but SEP-2575 says "Servers MUST NOT infer client capabilities from previous requests." This merge carries forward capabilities from initialize into subsequent draft requests. If a client intentionally drops a capability it previously declared, the server would still "remember" it via the merge. The IsStatefulSession() guard helps (stateless servers won't merge), but for stateful sessions a client can't un-declare a capability mid-session. Is this a known spec deviation that should be documented?

Comment on lines +55 to +67
[JsonConverter(typeof(TimeSpanMillisecondsConverter))]
public TimeSpan? TimeToLive { get; set; }

/// <inheritdoc />
/// <remarks>
/// Spec PR #2855 makes <c>cacheScope</c> a required field on <see cref="DiscoverResult"/>. The
/// server emits a safe default (<see cref="Protocol.CacheScope.Private"/>) on draft sessions
/// when the application has not set an explicit value.
/// </remarks>
[JsonPropertyName("cacheScope")]
[JsonConverter(typeof(CacheScopeConverter))]
public CacheScope? CacheScope { get; set; }
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remarks say spec PR #2855 makes ttlMs and cacheScope required fields on DiscoverResult, but they're declared as nullable (TimeSpan? / CacheScope?). The ConfigureDiscover handler always populates them with safe defaults, but since these are public set, a consumer building their own DiscoverResult could leave them null and produce a non-conformant response. Should these be required properties (like SupportedVersions, Capabilities, ServerInfo) to enforce the spec constraint at compile time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants